Plugin/TagFix_Maxspeed_AT #2506
Conversation
at:urban30 and at:urban40 have been superseded by at:city_limit30 and at:city_limit40 (which will be checked in a separate plugin).
…Austria Removed unused maxspeed types for Austria
Changed to commonly understood abbreviations
Abbreviations for Austrian provinces
…plugin/TagFix_Maxspeed_AT
…rt/osmose-backend into plugin/TagFix_Maxspeed_AT
plugins/TagFix_Maxspeed_AT.py
Outdated
| '''A speed limit type is given in `maxspeed:type` or `source:maxspeed`, but no speed limit is set in `maxspeed`.'''), | ||
| fix=T_( | ||
| '''Set `maxspeed` and `maxspeed:type` or `source:maxspeed` (but not both) as appropriate. | ||
| For a list of values, see table 'valid_maxspeed_types' in the source code below.'''), |
There was a problem hiding this comment.
I don't think the average user can read Python code. You can probably print it to the user if you wish, T_(...) can have a second argument with placeholder contents (T('abc {0}', 'AT:motorway') gives 'abc AT:motorway', where abc is translated and AT:motorway is not. Even better, point to the wiki.
There was a problem hiding this comment.
At the time there was no reliable information in the wiki. Meanwhile I've updated the wiki and will link to it.
plugins/TagFix_Maxspeed_AT.py
Outdated
| title=T_('Multiple speed limit types'), | ||
| detail=T_( | ||
| '''`maxspeed:type` and `source:maxspeed` are both set. This may cause confusion for mappers and data consumers, | ||
| especially if the values are different.'''), |
There was a problem hiding this comment.
Not sure what's wrong with maxspeed:type=AT:* + source:maxspeed=survey (or maybe survey;AT:*)
Also, if they are different and are linked to different values (say 130 and 50), "may cause confusion" is probably quite an understatement, while if the values are identical, it's at most redundant, not bad, is it?
There was a problem hiding this comment.
If maxspeed:type is set, it must be a valid speed type, and source:maxspeed can be anything except a speed type different from maxspeed:type (the code handles this).
If only source:maxspeed is set, things are more difficult. Out of 130.000 values, less than 50 are true comments (and many of those indicate situations to be resolved). So far, these would have been reported. I've changed the code to only report clearly invalid speed types. This will miss a couple of cases (e.g. values spearated with ;), but that's ok.
plugins/TagFix_Maxspeed_AT.py
Outdated
| detail=T_( | ||
| '''The speed limit in `maxspeed` is not consistent with the speed limit type in `maxspeed:type` or `source:maxspeed`.'''), | ||
| fix=T_( | ||
| '''Set `maxspeed` and/or `maxspeed:type`/`source:maxspeed` (but not both) as appropriate. For a list of values, |
There was a problem hiding this comment.
| '''Set `maxspeed` and/or `maxspeed:type`/`source:maxspeed` (but not both) as appropriate. For a list of values, | |
| '''Set either `maxspeed:type` or `source:maxspeed` (but not both), and `maxspeed`, as appropriate. For a list of values, |
Just to make sure it's 100% clear that the "(but not both)" does not refer to maxspeed in any situation.
plugins/TagFix_Maxspeed_AT.py
Outdated
| # Error: maxspeed type without maxspeed | ||
| if not maxspeed: | ||
| if maxspeed_type or source_maxspeed: | ||
| err.append({'class': 1, 'text': T_('Speed limit type without maxspeed')}) |
There was a problem hiding this comment.
Why nearly duplicate the title in text? Just make the title say maxspeed and remove the text, or make the text reflect the actual tag values (maxspeed:type or source:maxspeed) involved.
There was a problem hiding this comment.
Reworded to output speed limit type.
plugins/TagFix_Maxspeed_AT.py
Outdated
| # Error: maxspeed suspiciously low, probably 'walk'; needs verification | ||
| if maxspeed.isdigit(): | ||
| maxspeed_num = int(maxspeed) | ||
| if maxspeed_num < 10: |
There was a problem hiding this comment.
Note that < 5 is already covered by plugin Numeric.py
There was a problem hiding this comment.
Changed to exclude values < 5; expanded to cover values < 15 (which is the lowest possible zone) which are not explicitly signposted.
plugins/TagFix_Maxspeed_AT.py
Outdated
| # Error: maxspeed type doesn't match maxspeed | ||
| # except for types covered in TagFix_Maxspeed plugin and types without specific speed | ||
| if valid_type and valid_type not in {'AT:motorway', 'AT:trunk', 'AT:rural', 'AT:urban', 'sign', 'AT:zone', 'zone'}: | ||
| if maxspeed != self.valid_maxspeed_types.get(valid_type): |
There was a problem hiding this comment.
Rather than hardcoding the ones you set to '', you could consider checking if self.valid_maxspeed_types.get(valid_type) has a value.
plugins/TagFix_Maxspeed_AT.py
Outdated
| 'source:maxspeed': 'read it in the news'}, None) | ||
|
|
||
| # Error when maxspeed type without maxspeed | ||
| assert self.check_err(plugin.way(None, {'highway': 'secondary', 'maxspeed:type': 'sign'}, None)) |
There was a problem hiding this comment.
self.check_err doesn't need an assert
Co-authored-by: Famlam <[email protected]>
Removed failing test case (supposed to test for out-of-country data)
Reworded detail text as per osmose-qa#2506 (comment)
|
Your comments suggest code changes have been made but I don't see them here yet, possibly you forgot to push them live? |
|
I comment as I fix the problems, but push after final testing. Should be ready later today. Thanks for the detailed review. |
The plugin contains a number of tests focusing on
maxspeed,maxspeed:typeandsource:maxspeedspecific to Austria. It is by no means complete, but will do until further community decisions have been taken.I've tested the logic locally, but have no Osmose dev environment. I need ids for 6 issue classes (currently numbered 1 to 6) and integration support. Test cases are provided, but have never run (I used a local test harness to feed data). I'd like to provide a German translation following successful integration.
#2503